Move folding & visiting traits into type library #107712

eggyal · 2023-02-06T01:21:00Z

rustbot · 2023-02-06T01:21:10Z

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured in abstract_const.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

eggyal · 2023-02-06T08:58:03Z

compiler/rustc_type_ir/src/lib.rs

+pub trait OuterExclusiveBinder {
+    fn outer_exclusive_binder(self) -> DebruijnIndex;
+}
+
+pub trait BoundAtOrAboveBinder {
+    fn bound_at_or_above_binder(self, index: DebruijnIndex) -> bool;
+}
+
+pub trait BoundIndex {
+    fn bound_index(&self) -> Option<DebruijnIndex>;
+}
+
+pub trait Flags {
+    fn flags(self) -> TypeFlags;
+}


One alternative to these traits (and the refactorings of HasEscapingVarsVisitor and HasTypeFlagsVisitor that they enable) would be to project those two visitors from the Interner in order that they remain defined/implemented solely for TyCtxt (and so continue using the existing inherent methods).

bjorn3 · 2023-02-06T11:20:10Z

compiler/rustc_codegen_cranelift/src/common.rs

@@ -336,7 +336,7 @@ impl<'tcx> HasTargetSpec for FunctionCx<'_, '_, 'tcx> {
 impl<'tcx> FunctionCx<'_, '_, 'tcx> {
    pub(crate) fn monomorphize<T>(&self, value: T) -> T
    where
-        T: TypeFoldable<'tcx> + Copy,
+        T: TypeFoldable<TyCtxt<'tcx>> + Copy,


Could there be a trait alias for TypeFoldable filling in TyCtxt in rustc_middle? That would significantly reduce the diff.

Wasn't aware of trait aliases! Good idea, will look into it.

Okay so, whilst a trait alias can be used in bounds such as these, it can't be used to implement the trait: so we'll still need impl<'tcx> TypeFoldable<TyCtxt<'tcx>> for ... followed by ... where T: TypeFoldableAlias<'tcx>—and hence we'd still end up modifying each of these bounds to refer to the alias instead, unless the alias is named TypeFoldable (which I think would be very confusing unless the trait itself is renamed something else?). So the diff will likely remain similar, but the boilerplate would be slightly reduced.

I think it's fine to have a generic TypeFoldable that is almost never used and one that is used everywhere else. We do a similar thing for Ty after all. It's just that type aliases are stable.

I created another branch with an alternative implementation using trait aliases, but as suspected the impact on the overall diff is not so significant: +1494-1309 across 99 files versus this PR's +1709-1434 across 112 files.

I'm happy to use/force-push that if it's preferred, but IMHO defining e.g. TypeFoldable to mean different things depending on whether the alias or the underlying trait are brought into scope is pretty confusing (and all the existing reexports certainly don't help to keep track of what each name means in a given context). In particular for each trait, that branch now has at very least the following:

rustc_type_ir::fold::TypeFoldable // (1): actual trait definition rustc_middle::ty::fold::TypeFoldable // (2): re-export of (1) rustc_middle::ty::ir::TypeFoldable // (3): re-export of (2) rustc_middle::ty::fold::ctxt::TypeFoldable // (4): type alias definition rustc_middle::ty::TypeFoldable // (5): re-export of (4)

And the codebase variously uses each of the above in different and inconsistent ways. Certainly this could (and probably should) be rationalised, but fundamentally I think the naming conflict is unhelpful.

Incidentally, you may note that branch also removes some previously required imports from scope: this is a direct result of using trait aliases, which erroneously bring supertraits into scope (reported #107747). Once that bug is resolved, many of those imports will need to be re-added.

Thanks! ❤️

On reflection, I'm not sure there's any benefit in blanket implementing each (existing) middle trait (parameterised by 'tcx) for all types implementing the respective ir trait (parameterised by TyCtxt<'tcx>), because generic methods in the middle traits (e.g. TypeVisitable::visit_with) are parameterised by a type that must implement some other middle trait (e.g. V: TypeVisitor<'tcx>) and, in delegating to the ir trait, the blanket implementation must pass a value of that generic type to its equivalent method (ir::TypeVisitable::visit_with), which requires that value also to implement the respective ir trait (ir::TypeVisitor<TyCtxt<'tcx>>). However this cannot be guaranteed unless the middle traits are subtraits of the ir traits, thus always requiring that the ir traits be implemented and negating much benefit of retaining the middle traits; any remaining ergonomic benefit is then negated by ambiguities that such supertraits introduce when resolving associated items.

Hmm... I see. I'm assuming trait aliases won't allow impls TraitAlias for Type, thus leading to the branch you linked that still references TypeVisitable<TyCtxt<'tcx>> almost as often as before.

I'm convinced to reopen this PR, even if it's a bit annoying to have to do that all over the place.

Thanks for doing the experiment. Please add this information to the PR so that it's clear why this design was chosen

Indeed, you cannot (currently?) impl TraitAlias for ....

Will open a new PR (am looking into some other changes anyway) and provide the detail as suggested. Thanks!

eggyal · 2023-02-06T11:30:54Z

Having only just read the types team announcement, I'm not sure whether this PR is even desirable anymore? Does it fit into the new plan?

oli-obk · 2023-02-06T11:32:22Z

I believe it is orthogonal to any other projects we have. Making the compiler's crates depend less on the TyCtxt directly is always a good thing for reusability and testability of these datastructures. I still nominated it for T-types discussion to give you a definite answer

bjorn3 · 2023-02-06T11:56:23Z

And even if chalk is gone, rust-analyzer still needs a trait solver. It would be nice if it can reuse the same one rustc will use in the future.

eggyal · 2023-02-07T11:23:15Z

Force-push to ff87b7a modified some of the intermediate commits, but no change to the final result.

…e_lib_with_trait_alias, r=oli-obk Move folding & visiting traits into type library This is a rework of rust-lang#107712, following feedback on that PR. In particular, this version uses trait aliases to reduce the API churn for trait consumers. Doing so requires a workaround for rust-lang#107747 until its fix in rust-lang#107803 is merged into the stage0 compiler; this workaround, which uses conditional compilation based on the `bootstrap` configuration predicate, sits in dedicated commit b409329 for ease of reversion. The possibility of the `rustc_middle` crate retaining its own distinct versions of each folding/visiting trait, blanket-implemented on all types that implement the respective trait in the type library, was also explored: however since this would necessitate making each `rustc_middle` trait a subtrait of the respective type library trait (so that such blanket implementations can delegate their generic methods), no benefit would be gained. r? types

eggyal added 4 commits February 6, 2023 01:14

Refactor functions used by HasEscapingVarsVisitor

bea94aa

Refactor functions used by HasTypeFlagsVisitor

e0767d9

Refactor default TypeVisitable::error_reported

2eb4609

Project Binder and Predicate from Interner trait

f92b450

rustbot assigned jackh726 Feb 6, 2023

eggyal commented Feb 6, 2023

View reviewed changes

bjorn3 reviewed Feb 6, 2023

View reviewed changes

oli-obk added the I-types-nominated Nominated for discussion during a types team meeting. label Feb 6, 2023

eggyal added 4 commits February 7, 2023 10:35

Make visiting generic over the interner

8fcd967

Make folding generic over the interner

8754bd3

Move visiting traits to type library

59ee434

Move folding traits to type library

ff87b7a

eggyal force-pushed the move_fold_visit_traits_to_type_lib branch from 3e5b6f3 to ff87b7a Compare February 7, 2023 11:21

eggyal closed this Feb 7, 2023

eggyal deleted the move_fold_visit_traits_to_type_lib branch February 7, 2023 13:32

eggyal mentioned this pull request Feb 11, 2023

Move folding & visiting traits into type library #107924

Merged

jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move folding & visiting traits into type library #107712

Move folding & visiting traits into type library #107712

eggyal commented Feb 6, 2023

rustbot commented Feb 6, 2023

eggyal Feb 6, 2023 •

edited

Loading

bjorn3 Feb 6, 2023

eggyal Feb 6, 2023

eggyal Feb 6, 2023

oli-obk Feb 6, 2023

eggyal Feb 7, 2023 •

edited

Loading

oli-obk Feb 7, 2023

eggyal Feb 8, 2023 •

edited

Loading

oli-obk Feb 8, 2023

oli-obk Feb 8, 2023 •

edited

Loading

eggyal Feb 8, 2023

eggyal commented Feb 6, 2023 •

edited

Loading

oli-obk commented Feb 6, 2023 •

edited

Loading

bjorn3 commented Feb 6, 2023 •

edited

Loading

eggyal commented Feb 7, 2023

Move folding & visiting traits into type library #107712

Move folding & visiting traits into type library #107712

Conversation

eggyal commented Feb 6, 2023

rustbot commented Feb 6, 2023

eggyal Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

bjorn3 Feb 6, 2023

Choose a reason for hiding this comment

eggyal Feb 6, 2023

Choose a reason for hiding this comment

eggyal Feb 6, 2023

Choose a reason for hiding this comment

oli-obk Feb 6, 2023

Choose a reason for hiding this comment

eggyal Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk Feb 7, 2023

Choose a reason for hiding this comment

eggyal Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk Feb 8, 2023

Choose a reason for hiding this comment

oli-obk Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

eggyal Feb 8, 2023

Choose a reason for hiding this comment

eggyal commented Feb 6, 2023 • edited Loading

oli-obk commented Feb 6, 2023 • edited Loading

bjorn3 commented Feb 6, 2023 • edited Loading

eggyal commented Feb 7, 2023

eggyal Feb 6, 2023 •

edited

Loading

eggyal Feb 7, 2023 •

edited

Loading

eggyal Feb 8, 2023 •

edited

Loading

oli-obk Feb 8, 2023 •

edited

Loading

eggyal commented Feb 6, 2023 •

edited

Loading

oli-obk commented Feb 6, 2023 •

edited

Loading

bjorn3 commented Feb 6, 2023 •

edited

Loading